Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

helper/schema: Do not collapse TypeSet hash values when configuration block contains only Computed attributes #197

Merged
merged 1 commit into from
Feb 13, 2020
Merged

helper/schema: Do not collapse TypeSet hash values when configuration block contains only Computed attributes #197

merged 1 commit into from
Feb 13, 2020

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Oct 8, 2019

Reference: hashicorp/terraform-provider-aws#7198
Reference: hashicorp/terraform-provider-aws#10045
Reference: hashicorp/terraform-provider-aws#10339
Reference: hashicorp/terraform#22719

When a resource schema contains the following:

"config_block_attribute": {
	Type:     schema.TypeSet,
	Computed: true,
	Elem: &schema.Resource{
		Schema: map[string]*schema.Schema{
			"attribute1": {
				Type:     schema.TypeBool,
				Computed: true,
			},
			"attribute2": {
				Type:     schema.TypeString,
				Computed: true,
			},
		},
	},
},

The TypeSet hash values were previously all collapsed to the zero-value, which meant that multiple set entries were lost. Here we check that all of the attributes are not just Computed: true. If they are all Computed: true attributes, ignore the check for user-defined attributes to compute the hash value.

@bflad bflad added the bug Something isn't working label Oct 8, 2019
@bflad bflad mentioned this pull request Oct 8, 2019
Closed
@bflad bflad requested a review from a team October 8, 2019 15:45
@radeksimko radeksimko self-assigned this Oct 10, 2019
radeksimko
radeksimko approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the bug fix - this is much cleaner than workarounds proposed in the linked PRs. 😄

The only thing I'd be slightly worried about is where the HashResource function is used and in what context. e.g. We would not want spurious diffs to appear when a hash of a set item changes because a computed value has changed.

For that reason I'd like to double check with @hashicorp/terraform-core 👇

Can we be reasonably sure that the serialization logic is only used to guarantee uniqueness and not for diffing? I looked briefly at Set.HashEqual and Set.Equal, but I could not really trace the code using this + I reckon schema.HashResource can be used by providers themselves, which leaves me unsure and a bit confused.

@bflad If we get the 2nd 👍 I'd like to run all (or majority) of AWS acceptance tests with this patched SDK and check for any anomalies, due to the (perceived) sensitivity of this part of the codebase.
If we see no anomalies caused by this patch then I'm happy to merge it.

@radeksimko radeksimko requested a review from a team October 11, 2019 12:53
@apparentlymart
Copy link
Contributor

Unfortunately I think this is one of those things where the answer differs depending on whether we're talking protocol version 4 or 5.

In protocol version 4 (and thus Terraform 0.10/0.11), Terraform Core has no awareness of the idea of a set at all and just sees this as a flat map of key/value pairs. However, working in our favor is that Terraform 0.10 and 0.11 generally just trust entirely whatever diff is produced by the provider, because in protocol 4 the response is the diff. Therefore, as long as the provider (or rather, the SDK) is consistent with itself in how it interprets the prior state, this could be fine. I believe the legacy diffing logic uses the values for diffing and ignores the hash segments in the keys, but I'm not 100% sure so it'd be good to test it.

In protocol version 5, the result of PlanResourceChange is a planned new state, not a diff. (Terraform's CLI layer produces the visual diff at render time.) The hash values are an SDK implementation detail in this case, and are discarded by the shim layer before returning data to Terraform Core. Since the protocol 5 shim layer is required to re-constitute all of the flatmap hash keys for each new request, I believe it's essentially forced to be self-consistent: it constructs both the flatmap prior state and the flatmap proposed new state at the same time and then runs the legacy diffing logic between the two. I would expect both of them to get constructed using the current hash logic, because the hashes returned from the previous apply operation are not persisted in the state anyway.

There were some particularly messy cases when we were working through the shim behaviors during 0.12 development that I think we should be careful to test the behaviors of here:

  • When the nested schema.Resource for a Computed schema is empty, both when the container is a list and when it's a set (which lead to slightly different shimming logic). This situation is often an edge case for decisions like this "is everything computed?" because there's no "everything" to decide based on.
  • When the container is a set and everything inside the nested schema.Resource is computed. In that case, we'll need to make sure the set implementation doesn't end up collapsing everything into a single element for comparison even when there are values set.
  • Situations where we're performing an update and the update changes the value of one of the nested computed attributes, either at plan time or at apply time. The shim layer has a few cases where it has to create a temporary legacy diff using the old logic in order to replicate old behavior, and changes to the prior state values for computed attributes had a tendency to throw it off.

@radeksimko
Copy link
Member

@bflad Do you mind adding tests for the 3 cases Martin mentioned above? Alternatively you can leave it to me.

Assuming these tests are all passing and don't "uncover any skeletons" 😄 I would then continue by running AWS acceptance tests and ideally one more major provider, and if these pass then I'm happy to merge it.

@radeksimko radeksimko added the waiting-response Issues or pull requests waiting for an external response label Oct 17, 2019
@Didainius Didainius mentioned this pull request Nov 19, 2019
Merged
Didainius added a commit to vmware/terraform-provider-vcd that referenced this pull request Dec 4, 2019
@Didainius
Resource and data source vcd_edgegateway add network support for mu…
519ca43 
…ltiple subnets, sub-allocated pools and rate limits (#401)

This is a list of things it brings:
* Deprecates existing `external_networks` list of networks in favor of new `external_network` block which allows to specify one or more subnets inside the network and suballocate IPs from those ranges. Also deprecates `default_gateway_network` field. *Note*: as far as I tested all `external_network` block features work with simple edge gateway as well
* Adds additional `external_network_ips` computed field to get all IP addresses set on uplink gateway interfaces (in addition to default IP in `default_external_network_ip` field)
* Adds additional test `TestAccVcdEdgeGatewayParallelCreation` which aims to catch a reported error when two edge gateways are being attached to the same external network at the same time (so far I did not reproduce the error)
* Removed a limitation where field `distributed_routing` was not read due to older API version not returning it.
* Adds new field `fips_mode_enabled`  to enable FIPS mode (it must be enabled by vCD administrator in general settings to make it work, otherwise returns a human readable error). **Note** FIPS mode field is not being returned from API therefore it _does not support read_
* Adds support for rate limiting per "external network"

**Note**: It does not yet support update.

**Important**. There is one problem with data sources which should be solved by this PR hashicorp/terraform-plugin-sdk#197 but is not yet done. _The problem_ is that when a `TypeSet` contains only computed variables and there are multiple blocks, **only one** is stored. This situation is documented and tested in existing `TestAccVcdEdgeGatewayExternalNetworks` test
@Didainius Didainius mentioned this pull request Dec 17, 2019
Closed
@bflad bflad mentioned this pull request Jan 21, 2020
Closed
@paultyng paultyng added this to the v2.0.0 milestone Feb 6, 2020
@paultyng
Copy link
Contributor

paultyng commented Feb 6, 2020

Its probably safest to just merge this to v2, not sure the impact of merging to v1, do either of you @bflad / @radeksimko have thoughts on that?

@paultyng paultyng removed this from the v2.0.0 milestone Feb 6, 2020
@bflad
Copy link
Contributor Author

bflad commented Feb 6, 2020

@paultyng I think the AWS Provider folks are fine with this being in v2, since we will need to run the full testing anyways just to be safe.

@radeksimko
Copy link
Member

👍

@bflad @appilon
helper/schema: Do not collapse TypeSet hash values when configuration…
5de5396 
… block contains only Computed attributes

Reference: hashicorp/terraform-provider-aws#7198
Reference: hashicorp/terraform-provider-aws#10045
Reference: hashicorp/terraform-provider-aws#10339
Reference: hashicorp/terraform#22719

When a resource schema contains the following:

```go
"config_block_attribute": {
	Type:     schema.TypeSet,
	Computed: true,
	Elem: &schema.Resource{
		Schema: map[string]*schema.Schema{
			"attribute1": {
				Type:     schema.TypeBool,
				Computed: true,
			},
			"attribute2": {
				Type:     schema.TypeString,
				Computed: true,
			},
		},
	},
},
```

The TypeSet hash values were previously all collapsed to the zero-value, which meant that multiple set entries were lost. Here we check that all of the attributes are not just `Computed: true`. If they are all `Computed: true` attributes, ignore the check for user-defined attributes to compute the hash value.
@appilon appilon force-pushed the b-helper-schema-TypeSet-hash-all-computed branch from 7780a0b to 5de5396 Compare February 13, 2020 18:14
@appilon appilon changed the base branch from master to version2 February 13, 2020 18:14
@appilon appilon merged commit ed1b325 into version2 Feb 13, 2020
@appilon appilon deleted the b-helper-schema-TypeSet-hash-all-computed branch February 13, 2020 18:19
@paultyng paultyng added this to the v2.0.0 milestone Feb 13, 2020
@ghost
Copy link

ghost commented Mar 15, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 15, 2020
@ghost ghost removed the waiting-response Issues or pull requests waiting for an external response label Mar 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radeksimko radeksimko radeksimko approved these changes

Assignees

@radeksimko radeksimko

Labels
bug Something isn't working
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@bflad @apparentlymart @radeksimko @paultyng @appilon